Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support python 3.12 #138

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Support python 3.12 #138

merged 3 commits into from
Oct 10, 2023

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Oct 2, 2023

The necessary functions are copied from cpython 3.12 Include/internal/pycore_long.h, which is private. This should make it easy to copy new versions if cpython internals change. For cpython < 3.12 I implemented equivalent functions.

All of this is placed in separate files pycore_long.pxd and pycore_long.h so it's easy to share between projects (fpylll, sagemath).

Tested in 64-bit and 32-bit x86.

Fixes: #137

@tornaria
Copy link
Contributor Author

tornaria commented Oct 6, 2023

See sagemath/sage#36407 (comment) for an explanation of the change.

@mkoeppe
Copy link

mkoeppe commented Oct 6, 2023

rebased

Comment on lines +23 to +24
gppath = shutil.which("gp")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what's "shutil" or "shutils" but I note that you use two different names.

Also: I didn't need this patch to successfully build cypari2 with python 3.12. What I PR is good enough (as in: I can build and fully test sagemath with it). If this is a problem, is not catched by the testsuite of cypari2 or sagemath. I know distutils is not available anymore, so maybe that means this file is not used at all? It's definitely not tested (as all tests pass for me IIRC).

PS: I find it intriguing the asymmetry that some sage developers can modify my PRs but I can't modify any PR not authored by me. Granted, that was a useful feature of trac, but it's not a standard feature of gh afaik. I don't mind you changing my PRs, just something to think about. I'm glad that you are working on getting releases for cypari2 (and the other pkgs we need).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it intriguing the asymmetry that some sage developers can modify my PRs but I can't modify any PR not authored by me.

One needs to be in the Maintainer role for the project. https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization
I am not sure if there is a clear protocol how and when to elevate developers to this role, but an email to sagemath-admins would probably help.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as PR author one can uncheck the box "Maintainers are allowed to edit this pull request." to remove unwelcome interference from maintainers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given sagemath/triage team (with @tornaria there) same rights here (and on few other sagemath/ repos) as on the main Sage repo. Hopefully this gives more rights to fix stuff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autogen/paths.py Outdated
Comment on lines 18 to 19
import shutils

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean in the import here

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It should be shutil.
https://docs.python.org/3/library/shutil.html

@dimpase
Copy link
Member

dimpase commented Oct 9, 2023

I presume the CI errors are not from our code.

@mkoeppe
Copy link

mkoeppe commented Oct 9, 2023

rebased again

@kiwifb
Copy link
Member

kiwifb commented Oct 9, 2023

I must say I am bothered by the presence of pycore_long.h and pycore_long.pxd. They appear here and in sagemath/sage#36407 identically. One we are surfacing a private header which is fraught with danger, upstream should make an interface available. And two this is duplication of code which can lead to things getting out of sync. But the only alternative I see, is spinning it on its own which comes with its own problems.

@kiwifb
Copy link
Member

kiwifb commented Oct 9, 2023

Provided that pplpy and fpylll do not need this stuff as well, could sage make us of a copy coming from cpypari2 here rather than having its own?

@tornaria
Copy link
Contributor Author

tornaria commented Oct 9, 2023

I must say I am bothered by the presence of pycore_long.h and pycore_long.pxd. They appear here and in sagemath/sage#36407 identically. One we are surfacing a private header which is fraught with danger, upstream should make an interface available. And two this is duplication of code which can lead to things getting out of sync. But the only alternative I see, is spinning it on its own which comes with its own problems.

As I mentioned in sagemath/sage#36407 (comment):
"Arguably this should be moved to cython itself (there is a cython module cpython.longintrepr) but for now this is the simpler solution."

But python 3.12 is shipping and we need sagemath working. This makes it work, it's needed in cypari2 and in fpylll.

I'm also bothered by code "duplication". For now, let's think of this as "vendoring in" the module, that's why I stressed that I pushed "identical" files. The whole thing is structured in such a way that (a) users (cypari2, fpylll, sagemath) only care about the interface as specified in pycore_long.pxd; (b) the implementation in pycore_long.h can be extended to python 3.13 if/when need arises, without having to alter anything else (other than copying the new version of pycore_long.h); (c) it seems plausible to add this interface to cpython.longintrepr in cython.

@mkoeppe
Copy link

mkoeppe commented Oct 9, 2023

+1 on duplicating it for now and trying to push it into Cython as a follow up.

-1 on trying to have Sage import it from cypari2; that would be problematic in the context of modularization.

@kiwifb
Copy link
Member

kiwifb commented Oct 9, 2023

I am OK with whatever you want to do so long as we have had the conversation. It is a case of not flying blind.

@mkoeppe
Copy link

mkoeppe commented Oct 10, 2023

@videlec @dimpase Please review and merge

@dimpase dimpase merged commit b178c0a into sagemath:master Oct 10, 2023
95 of 106 checks passed
@tornaria tornaria deleted the py312 branch October 10, 2023 12:18
J08nY added a commit to J08nY/cypari2 that referenced this pull request Jan 23, 2024
This was introduced by accident in sagemath#138.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with Python 3.12
4 participants